Implementation of multiple revisions without revisions map #6620
+1,206
−139
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
… operation Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
…f "master" Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Dr Marco De La Pierre <[email protected]>
Signed-off-by: Dr Marco De La Pierre <[email protected]>
Signed-off-by: Dr Marco De La Pierre <[email protected]>
Signed-off-by: Dr Marco De La Pierre <[email protected]>
Signed-off-by: Dr Marco De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
…ate method ; plus related updates across codebase
Signed-off-by: jorgee <[email protected]>
Signed-off-by: Jorge Ejarque <[email protected]>
Signed-off-by: jorgee <[email protected]>
…re repo for default branch get branches and tags Signed-off-by: jorgee <[email protected]>
Signed-off-by: jorgee <[email protected]>
Signed-off-by: jorgee <[email protected]>
…lones of the same commit Signed-off-by: jorgee <[email protected]>
This was referenced Dec 1, 2025
Contributor
Author
|
Find bellow the directory tree for a project with the bare repo and two revisions (commit 451ebdand and master branch).
|
pditommaso
requested changes
Dec 1, 2025
Member
pditommaso
left a comment
pditommaso
left a comment
There was a problem hiding this comment.
Can briefly summarise what's the benefits of this approach vs revisions map ?
Comment on lines
60
to
64
| List<AssetManager> dropList = [] | ||
| if ( allRevisions ) { | ||
| def revManager = new AssetManager(args[0]) | ||
| revManager.listRevisions().each { rev -> | ||
| if( rev == DEFAULT_REVISION_DIRNAME ) | ||
| rev = null | ||
| if( !revManager.localRootPath.exists() ) { | ||
| throw new AbortOperationException("No match found for: ${revManager.getProjectWithRevision()}") | ||
| } | ||
| revManager.listCommits().each { rev -> |
Member
There was a problem hiding this comment.
Let's try reducing the cyclomatic complexity of this method, splitting in smaller self-describing private methods.
Comment on lines
61
to
92
| if( moreDetailed ) | ||
| detailed = true | ||
| if( detailed && allRevisions ) { | ||
| all.each { | ||
| println(" $it") | ||
| def revManager = new AssetManager(it) | ||
| revManager.listRevisionsAndCommits().each { k, v -> | ||
| if( !moreDetailed ) | ||
| v = v.substring(0, 10) | ||
| println(" $v $k") | ||
| } | ||
| } | ||
| } else if( allRevisions ) { | ||
| all.each { | ||
| println(" $it") | ||
| def revManager = new AssetManager(it) | ||
| revManager.listRevisions().each { | ||
| println(" $it") | ||
| } | ||
| } | ||
| } else if( allCommits ) { | ||
| all.each { | ||
| println(" $it") | ||
| def revManager = new AssetManager(it) | ||
| revManager.listCommits().each { println(" $it") } | ||
| } | ||
| } else { | ||
| all.each { println(" $it") } | ||
| } | ||
| } else if( allCommits ) { | ||
| all.each{ | ||
| println(" $it") | ||
| def revManager = new AssetManager(it) | ||
| revManager.listCommits().each{ println(" $it") } | ||
| } | ||
| } else { | ||
| all.each{ println(" $it") } | ||
| } | ||
| } | ||
|
|
||
| } |
Member
There was a problem hiding this comment.
As above, try to make this logic more compact following "Reveal intention" pattern. https://claude.ai/share/32835bb7-8395-431e-a73f-53797de9aaf6
Contributor
Author
|
The main benefit is removing the revisions map. It reduces the code to maintain as we removed all the functions to create, update, prune the map for every operation. It also avoids information duplication. What was stored in the map is currently in the bare repo and with per-revision fetch, there is no difference in the behavior. The only case were it is less efficient is in the detailed |
Member
|
Agree on keep a single source of truth and removing duplicate commands |
Signed-off-by: jorgee <[email protected]>
…n MultiRevisionAssetManager Signed-off-by: jorgee <[email protected]>
jorgee
changed the base branch from
add/mult_revisions_jun_revision_map
to
master
This was
linked to
issues
Dec 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative for #5089
This pull request refactors revision and commit management for pipeline assets, streamlining how revisions are tracked and handled across CLI commands and internal logic. The main changes include removing the legacy revision map mechanism, updating CLI flags for clarity and consistency, and improving concurrency safety when cloning repositories. These updates simplify the codebase and improve reliability when managing pipeline versions.
Revision and Commit Management Refactor
REVISION_MAP,DEFAULT_REVISION_DIRNAME) fromAssetManager, including related methods (getRevisionMap,revisionToCommitWithMap,updateRevisionMap, etc.). Revision tracking now relies directly on branch/tag and commit information from the repository. [1] [2] [3] [4] [5]CLI Improvements
-a, -all-revisionsflag to-a, -allinCmdDropfor clarity, and updated help descriptions accordingly.-d, -deepflag fromCmdPullandCmdRun, and marked it as deprecated in documentation for future removal. [1] [2] [3] [4]Concurrency and Reliability
AssetManager.createSharedCloneto prevent concurrent clones of the same commit, ensuring safe and reliable asset downloads. [1] [2]Codebase Cleanup
CmdDrop.groovy,CmdList.groovy,CmdPull.groovy). [1] [2] [3]These changes collectively modernize revision tracking and asset management, making the system more robust and maintainable.